[jsweep] Clean action_input_utils.cjs#33933
Conversation
Improved JSDoc to explicitly document that the underscore form has precedence over the hyphen form, clarifying the OR chain behavior that the existing tests already verify. File was already in excellent shape: - Modern ES6+ JavaScript - Full TypeScript type checking enabled (@ts-check) - Comprehensive test coverage (13 tests covering all edge cases) - No code smells or unnecessary try/catch blocks Changes: - Enhanced JSDoc documentation for precedence behavior Validation: ✅ npm run format:cjs - passed ✅ npm run lint:cjs - passed ✅ npm run typecheck - passed ✅ npm run test:js - 13/13 tests passed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #33933 does not have the 'implementation' label and has 0 new lines of code in business logic directories (well below the 100-line threshold). |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in PR #33933. The PR only modified actions/setup/js/action_input_utils.cjs (a production file). Test Quality Sentinel analysis skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. No action needed: PR #33933 is already merged and closed (merged at 2026-05-22T05:20:17Z). Cannot add review comments to closed PRs. The PR added documentation claiming whitespace-only INPUT values have precedence, but this is technically inaccurate: empty string INPUT values (falsy) fall through to the hyphen form, while whitespace-only values (truthy) are used but then trimmed to empty string. However, since the PR is merged, no review action can be taken. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Updates the JSDoc for getActionInput() to explicitly document the existing precedence rules between underscore and hyphen GitHub Actions input environment variable forms.
Changes:
- Documented that
INPUT_<NAME>(underscore form) takes precedence overINPUT_<NAM-E>(hyphen form), including the whitespace-only behavior. - Clarified that the hyphen form is only consulted when the underscore form is absent or an empty string.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/action_input_utils.cjs | JSDoc clarification of underscore-vs-hyphen env var precedence for action inputs |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — requesting changes due to documentation inaccuracy.
📋 Key Finding
Critical Issue
Documentation contradicts implementation and test suite
The new JSDoc claims that the hyphen form is checked "if the underscore form is absent or empty string," but this is incorrect. The implementation uses JavaScript's || operator, which means:
- ✅ Empty string
""→ falls back to hyphen form (truthy check fails) - ❌ Whitespace
" "→ does NOT fall back (truthy, so||short-circuits)
The test suite explicitly documents this edge case at line 30–36:
it("does not fall back to hyphen form when underscore form is whitespace-only (whitespace is truthy)", ...)Why This Matters
Documentation that contradicts the code creates confusion and maintenance debt. Future developers might:
- Trust the docs and be surprised by whitespace behavior
- "Fix" the code to match the docs, breaking existing workflows
- Waste time debugging why their inputs don't work as documented
✅ Positive Highlights
- Excellent test coverage (13 tests for a 3-line function)
- Test names read as specifications — very clear intent
- Good motivation for adding the documentation (behavior was implicit)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 517.5K
| * both forms ensures the value is resolved regardless of the runner version. | ||
| * | ||
| * The underscore form has precedence: if `INPUT_<NAME>` exists (even as whitespace), | ||
| * it is used. The hyphen form is only checked if the underscore form is absent or |
There was a problem hiding this comment.
[/grill-with-docs] Documentation inaccuracy: whitespace-only values do NOT fall back to hyphen form.
❌ The issue
The new documentation states:
The underscore form has precedence: if
INPUT_<NAME>exists (even as whitespace), it is used. The hyphen form is only checked if the underscore form is absent or empty string.
This contradicts the actual behavior. Whitespace-only values are truthy, so the || operator short-circuits and never checks the hyphen form. The test suite explicitly documents this:
it("does not fall back to hyphen form when underscore form is whitespace-only (whitespace is truthy)", () => {
vi.stubEnv("INPUT_JOB_NAME", " ");
vi.stubEnv("INPUT_JOB-NAME", "real-value");
expect(getActionInput("JOB_NAME")).toBe(""); // Returns "", not "real-value"
});✅ Suggested fix
Replace lines 12–14 with:
* The underscore form has precedence: if `INPUT_<NAME>` is truthy (including
* whitespace-only strings), it is used and the result is trimmed. The hyphen form
* is only checked if the underscore form is absent (undefined) or empty string ("").Or, to match the test name more precisely:
* Precedence: The underscore form (`INPUT_<NAME>`) is checked first using the `||`
* operator. If it is truthy (even whitespace like `" "`), it is used and trimmed.
* The hyphen form is only checked if the underscore form is falsy (undefined, null, or "").
Summary
Reviewed and improved documentation for
action_input_utils.cjs.File Status
This file was already in excellent production-ready shape:
@ts-check)Changes Made
Enhanced JSDoc documentation to explicitly document precedence behavior:
INPUT_<NAME>) has precedence over the hyphen form (INPUT_<NAM-E>)Context
Execution context: Node.js / GitHub Actions runtime
Test coverage: 13 test cases covering:
Validation Results
All validation checks passed:
✅ Formatting:
npm run format:cjs- passed (no changes needed)✅ Linting:
npm run lint:cjs- passed (no issues)✅ Type checking:
npm run typecheck- passed (strict type safety)✅ Tests:
npm run test:js- 13/13 tests passed (100%)Impact
This is a documentation-only improvement. No functional changes to the code.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.